Perf/bound review latency - #803
Conversation
|
|
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if opts.maxTokens > 0 { | ||
| cc.Template.MaxTokens = opts.maxTokens | ||
| } |
There was a problem hiding this comment.
[bug · high]
Bug: When maxTools is 0 (the default), the template's MaxToolRequestTimes is never overridden. However, the template validation in loadCommonContext checks if t.MaxToolRequestTimes <= 0 and returns an error. If the embedded template has MaxToolRequestTimes = 0, this will fail validation even though the user didn't explicitly set the flag. The old logic (if maxTools > tpl.MaxToolRequestTimes) would only override when the user explicitly set a higher value, preserving the template's default. The new logic breaks this by treating 0 as "use template default" but the template might have 0, causing validation to fail.
| if maxTools > 0 { | ||
| tpl.MaxToolRequestTimes = maxTools | ||
| } |
There was a problem hiding this comment.
[bug · high]
Bug: Scan command behavior changed. The change from maxTools > tpl.MaxToolRequestTimes to maxTools > 0 breaks the scan command's documented behavior.
In scan_cmd.go:111, the scan command still uses the old "only raise" logic:
if opts.maxTools > scanTpl.MaxToolRequestTimes {
scanTpl.MaxToolRequestTimes = opts.maxTools
}But now loadCommonContext will override the review template's MaxToolRequestTimes whenever maxTools > 0, even if it's lower than the template default. This contradicts the scan flag's help text at line 177: "max tool call rounds per file; only takes effect when greater than template default".
The review command should also only raise the limit, not lower it, to match user expectations and the documented behavior.
Suggestion:
| if maxTools > 0 { | |
| tpl.MaxToolRequestTimes = maxTools | |
| } | |
| if maxTools > tpl.MaxToolRequestTimes { | |
| tpl.MaxToolRequestTimes = maxTools | |
| } |
…d-review-latency # Conflicts: # cmd/opencodereview/shared_flags.go
2 participants
Description
Type of Change
How Has This Been Tested?
make testpasses locallyChecklist
go fmt,go vet)Related Issues